Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/sub pairs #3

Closed
wants to merge 35 commits into from
Closed

Feature/sub pairs #3

wants to merge 35 commits into from

Conversation

gbjk
Copy link
Owner

@gbjk gbjk commented Feb 19, 2024

No description provided.

gbjk added 16 commits February 16, 2024 10:28
IsInit was basically the same as IsConnected.
Any time Connect was called both would be set to true.
Any time we had a disconnect they'd both be set to false
Shutdown() incorrectly didn't setInit(false)

SetProxyAddress simplified to only reconnect a connected Websocket.
Any other state means it hasn't been Connected, or it's about to
reconnect anyway.
There's no handling for IsConnecting previously, either, so I've wrapped
that behind the main mutex.
This allows for testing and avoids the repetition.
If each returned error is a error.New() you can never use errors.Is()
Testing just the last id doesn't feel very robust
This was spurred by looking at the setState call in trafficMonitor and
the effect on blocking and efficiency.
With the new atomic types in Go 1.19, and the small types in use here,
atomics should be safe for our usage. bools should be truly atomic,
and uint32 is atomic when the accepted value range is less than one byte/uint8 since
that can be written atomicly by concurrent processors.
Maybe that's not even a factor any more, however we don't even have to worry enough to check.
trafficMonitor had a check throttle at the end of the for loop to stop it just gobbling the (blocking) trafficAlert channel non-stop.
That makes sense, except that nothing is sent to the trafficAlert channel if there's no listener.
So that means that it's out by one second on the trafficAlert, because any traffic received during the pause is doesn't try to send a traffic alert.

The unstopped timer is deliberately leaked for later GC when shutdown.
It won't delay/block anything, and it's a trivial memory leak during an infrequent event.

Deliberately Choosing to recreate the timer each time instead of using Stop, drain and reset
Fix race on changing trafficCheckInterval
@gbjk gbjk force-pushed the feature/sub_pairs branch 5 times, most recently from 33e3860 to 00a56cc Compare February 19, 2024 07:22
@gbjk gbjk force-pushed the feature/websocket_state branch from c6dc10c to 7443745 Compare February 20, 2024 09:13
trafficMonitor does not need to set the connection to be connected.
Connect() does that. Anything after that should result in a full
shutdown and restart. It can't and shouldn't become connected
unexpectedly, and this is most likely a race anyway.

Also dropped trafficCheckInterval to 100ms to mitigate races of traffic
alerts being buffered for too long.
@gbjk gbjk force-pushed the feature/websocket_state branch from 7443745 to 2f6bfc0 Compare February 20, 2024 09:16
gbjk added 5 commits February 21, 2024 14:08
Retain the .msg field of a go fmt.Errorf .msg field returned by .Error()
when wrapping multiple errors.
This fixes a situation where a nested stack of errors would lose
formatting information, which is often used to supply identifying
context.
e.g.
```
err = fmt.Errorf("%w `%s`: %w", errParsingField, fieldName,
parsingError)
errs = common.AppendError(errs, err)
```

This isn't really an issue with our implementation; Calling Unwrap() on a
fmt.Errorf() which returns a wrapErrors will lose that formatting.
Our issue was that we were using just Unwrap() to bind together our
chain-of-custody.
If the exchange passed in already has a websocket, don't clobber it
instead of errChannelAlreadySubscribed
@gbjk gbjk force-pushed the feature/sub_pairs branch 5 times, most recently from f62e3ce to dcfd5a5 Compare February 21, 2024 07:48
gbjk added 3 commits February 22, 2024 10:36
Given that some subscriptions have multiple pairs, support that as the
standard.
Except Kraken, which needs atomicity to not collide with upcoming work
Note: This is a naieve implementation because we want to rebase the
kraken websocket rewrite on top of this
@gbjk gbjk force-pushed the feature/sub_pairs branch from c7fa2b4 to 0fbb7cd Compare February 22, 2024 03:36
gbjk added 2 commits February 22, 2024 10:37
We deliberately use Equal over Len to avoid spamming the contents of large Slices
@gbjk gbjk force-pushed the feature/sub_pairs branch 8 times, most recently from 1b6804b to 7e58e5f Compare February 23, 2024 03:06
@gbjk gbjk force-pushed the feature/sub_pairs branch from 7e58e5f to 579ec55 Compare February 23, 2024 03:21
@gbjk gbjk force-pushed the feature/websocket_state branch 2 times, most recently from 0b03808 to 9aca06e Compare February 23, 2024 06:01
@gbjk gbjk deleted the branch feature/websocket_state February 23, 2024 08:00
@gbjk gbjk closed this Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant